perf(TreeView): make rows paint-safe and reduce hover/focus invalidation#7899
perf(TreeView): make rows paint-safe and reduce hover/focus invalidation#7899mattcosta7 wants to merge 4 commits into
Conversation
🦋 Changeset detectedLatest commit: 21ea23c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
the
integration-tests: recommended
|
mattcosta7
force-pushed
the
treeview-paint-safe-perf
branch
from
a12402c to
ddb1910
Compare
…ver/focus invalidation
- Add 'overflow-clip-margin: var(--base-size-8)' to '.TreeViewItem' and
'.TreeViewItemContainer' so the current-item indicator (positioned at
'left: -8px' of the row container) remains visible when a consumer applies
'contain: paint' to the row, or when the documented 'containIntrinsicSize'
prop triggers 'content-visibility: auto' (which implies paint containment)
on the container. The property is a no-op when no paint containment is
active, so default rendering is unchanged. Safari 17 silently ignores the
property and keeps the pre-existing clipped-indicator behavior under
consumer-side 'contain: paint' (no regression from prior versions); Safari
18+ supports it, as does 'content-visibility: auto'.
- Replace ':has(.TreeViewItemSkeleton):hover' with a positive '[data-loading]'
marker emitted on the placeholder row's <li> by 'LoadingItem' via a
module-private 'LoadingPlaceholderContext'. No public-API change; avoids the
broad ':has()' invalidation cost across every row in large trees.
- Replace the root-scope ':hover'/':focus-within' descendant selectors on
level lines with an inherited '--tree-line-color' custom property toggled on
the root <ul>. Same UX; one property toggle on one element instead of
selector re-matching against every '.TreeViewItemLevelLine' descendant.
- Fix unitless 'outline-offset: -2' in the forced-colors focus-ring fallback
that browsers were silently dropping (so forced-colors users now actually
get a focus indicator on tree items).
- Make 'grid-template-columns' declare the 'trailingAction' track explicitly
('auto') so it matches the 5-area 'grid-template-areas' declaration.
mattcosta7
force-pushed
the
treeview-paint-safe-perf
branch
from
ddb1910 to
59c4714
Compare
|
🤖 Lint issues have been automatically fixed and committed to this PR. |
Removed comments regarding Safari 17 behavior and stylelint rules.
There was a problem hiding this comment.
Pull request overview
Targeted performance and correctness fixes to TreeView styling. Rows are made paint-safe so the current-item indicator survives contain: paint / content-visibility: auto, the skeleton hover-suppression :has() selector is replaced with a positively-marked data-loading attribute, and nesting-line hover toggling is moved from a descendant selector to a single inherited custom property. Also fixes a unitless outline-offset: -2 in the forced-colors focus fallback and makes the trailing-action grid column explicit.
Changes:
- Add
overflow-clip-margin: var(--base-size-8)on.TreeViewItemand.TreeViewItemContainer; fix unitlessoutline-offset; makegrid-template-columnsdeclare thetrailingActiontrack explicitly. - Replace
:has(.TreeViewItemSkeleton):hoverwith a module-privateLoadingPlaceholderContextthat setsdata-loadingon the<li>, and key the CSS off that attribute. - Drive nesting indicator color through an inherited
--tree-line-colorcustom property toggled on the root<ul>instead of via:hover/:focus-withindescendant selectors; add a VRT story forcurrent+containIntrinsicSize.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/TreeView/TreeView.tsx | Adds LoadingPlaceholderContext, consumes it in Item to emit data-loading, and wraps both LoadingItem return paths with the provider. |
| packages/react/src/TreeView/TreeView.module.css | Adds overflow-clip-margin, explicit auto trailing column, fixes outline-offset: -2px, replaces :has() skeleton selector with [data-loading], refactors level-line color to inherited --tree-line-color. |
| packages/react/src/TreeView/TreeView.features.stories.tsx | Adds CurrentItemWithContainIntrinsicSize story for VRT coverage of the previously-clipped indicator. |
| .changeset/treeview-indicator-paint-safe.md | Patch changeset summarizing the five changes. |
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 0
|
|
||
| &:where([data-omit-spacer='true']) .TreeViewItemContainer { | ||
| grid-template-columns: 0 0 0 1fr; | ||
| grid-template-columns: 0 0 0 1fr auto; |
There was a problem hiding this comment.
auto is the default, but it reads nicer imp when it's 5 columns defined for 5 columns
liuliu-dev
added
the
Canary Release
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/21960 |
mattcosta7
added
integration-tests: skipped manually
mattcosta7
added
integration-tests: recommended
|
Integration test results from github/github-ui PR:
CI check runs linting, type checking, and unit tests. Check the workflow logs for specific failures. Need help? If you believe this failure is unrelated to your changes, please reach out to the Primer team for assistance. |
Closes #
Five targeted improvements to `TreeView` style + structure aimed at making rows cheaper to render at scale and removing a latent bug in the documented `containIntrinsicSize` API. No visual, layout, or public-API changes at the default rendering — all changes are either invisible at the default state or behind a CSS containment property the consumer opts into.
Changelog
Changed
`. Same UX (lines fade in while the tree is interacted with on hover-capable devices, always visible on coarse pointers); the engine updates one property on one element instead of re-matching the descendant selector against every `.TreeViewItemLevelLine` in the tree.- Fixed unitless `outline-offset: -2` in the forced-colors focus-ring fallback. Browsers were silently dropping the declaration, so forced-colors users were getting no focus indicator on tree items.
- Made `grid-template-columns` declare the `trailingAction` track explicitly (`auto`) so it matches the 5-area `grid-template-areas` declaration on the same element. Clarity, no behavior change.
New
Removed
Nothing public.
Rollout strategy
Testing & Reviewing
VRT expectations: No change in default rendering. The one place a change could surface is forced-colors focus rings on tree items, which go from "nothing" (the silent `outline-offset: -2` drop) to a 2px inset `HighlightText` outline. That's an accessibility fix.
Internal-only note: this lets the github-ui escape hatch in `packages/diff-file-tree/DiffFileTree.module.css` (`content-visibility: visible; contain: none;` for `[aria-current='true']` and `:focus-visible`) be removed once this lands.
Merge checklist